Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

types: fix overflow check of types/convert.go::floatStrToIntStr #11114

Merged
merged 22 commits into from
Jul 16, 2019

Conversation

H-ZeX
Copy link
Contributor

@H-ZeX H-ZeX commented Jul 6, 2019

Signed-off-by: H-ZeX hzx20112012@gmail.com

What problem does this PR solve?

Fix overflow check of types/convert.go::floatStrToIntStr

Check List

  • Unit test
  • Integration test

@CLAassistant
Copy link

CLAassistant commented Jul 6, 2019

CLA assistant check
All committers have signed the CLA.

Signed-off-by: H-ZeX <hzx20112012@gmail.com>
Signed-off-by: H-ZeX <hzx20112012@gmail.com>
@codecov
Copy link

codecov bot commented Jul 6, 2019

Codecov Report

Merging #11114 into master will increase coverage by 0.3747%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             master     #11114        +/-   ##
================================================
+ Coverage   81.3186%   81.6933%   +0.3747%     
================================================
  Files           423        423                
  Lines         90368      91699      +1331     
================================================
+ Hits          73486      74912      +1426     
+ Misses        11564      11483        -81     
+ Partials       5318       5304        -14

@H-ZeX H-ZeX requested a review from lonng July 8, 2019 08:03
@bb7133 bb7133 added the type/enhancement The issue or PR belongs to an enhancement. label Jul 8, 2019
Copy link
Contributor

@tangenta tangenta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to add some tests for isOverflowInt64() to pass test-coverage-ci.

H-ZeX added 3 commits July 9, 2019 10:38
Signed-off-by: H-ZeX <hzx20112012@gmail.com>
Signed-off-by: H-ZeX <hzx20112012@gmail.com>
@H-ZeX
Copy link
Contributor Author

H-ZeX commented Jul 9, 2019

/run-all-tests

@H-ZeX
Copy link
Contributor Author

H-ZeX commented Jul 9, 2019

/rebuild

@H-ZeX
Copy link
Contributor Author

H-ZeX commented Jul 9, 2019

/run-integration-tests

sc.AppendWarning(ErrOverflow.GenWithStackByArgs("BIGINT", oriStr))
return validFloat[:eIdx], nil
}
intCnt += exp
if intCnt <= 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intCnt will never be negative according to the L448.
Prefer to:

if intCnt > 0 {
	extraZeroCount := ...
} else {
	...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

intCnt>=0, exp>=0, however, if exp=I64::MAX, intCnt>0, then their sum will overflow and get negative.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

intCnt>=0, exp>=0, however, if exp=I64::MAX, intCnt>0, then their sum will overflow and get negative.

If intCnt < 0 will early return in L451.

types/convert.go Outdated
// (exp + incCnt) overflows MaxInt64.
intCnt += exp
// intCnt will < 0 when overflow
if intCnt < 0 || intCnt > 20 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The StrToUint will use this function to extract valid int string prefix, so I think we cannot limit the length to 20.

types/convert.go Outdated
intStr = string(digits) + strings.Repeat("0", extraZeroCount)
}
if isOverflowInt64(intStr) {
Copy link
Contributor

@lonng lonng Jul 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should check the overflow here because the duty of this function is to extract the valid int string prefix from a float string (the strconv.ParseInt will take charge of this, duplicate-check will cause performance issue).

Copy link
Contributor

@lonng lonng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

Copy link
Contributor

@tangenta tangenta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@lonng lonng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

lonng
lonng previously approved these changes Jul 11, 2019
@lonng
Copy link
Contributor

lonng commented Jul 11, 2019

/run-all-tests

@H-ZeX H-ZeX changed the title types: fix overflow check of types/convert.go::floatStrToIntStr [DNM]types: fix overflow check of types/convert.go::floatStrToIntStr Jul 11, 2019
Signed-off-by: H-ZeX <hzx20112012@gmail.com>
@H-ZeX H-ZeX changed the title [DNM]types: fix overflow check of types/convert.go::floatStrToIntStr types: fix overflow check of types/convert.go::floatStrToIntStr Jul 11, 2019
@H-ZeX H-ZeX removed the status/DNM label Jul 11, 2019
Copy link
Contributor

@lonng lonng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lonng
Copy link
Contributor

lonng commented Jul 16, 2019

/run-all-tests

@lonng lonng merged commit 3ec46b0 into master Jul 16, 2019
@lonng lonng deleted the fix_overflow_check branch July 16, 2019 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants